Skip to content

Optimize full-text search's SecurityQuery - #8041

Open
labkey-adam wants to merge 7 commits into
developfrom
fb_search_security_query
Open

labkey-adam wants to merge 7 commits into
developfrom
fb_search_security_query

Conversation

@labkey-adam

Copy link
Copy Markdown
Contributor

Rationale

Permission checking done by SecurityQuery is slow with many containers. Details here: https://github.com/LabKey/internal-issues/issues/819

@labkey-susanh labkey-susanh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done no testing, but the changes seem like good optimizations

Comment thread search/src/org/labkey/search/model/SecurityQuery.java Outdated
… instead of passing an output parameter to the method. Also, simplify the categoriesByPermissions part: return a simple Map instead of a MultiValuedMap, since that's all callers need.
@labkey-jeckels

Copy link
Copy Markdown
Contributor

The profile looks the same and it's still slow. It's spending all of its time in SearchCategory.getPermittedContainerIds() which is marked with @Deprecated // TODO: Remove after testing.

Am I supposed to make a code change as part of testing?

@labkey-adam

labkey-adam commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

The profile looks the same and it's still slow. It's spending all of its time in SearchCategory.getPermittedContainerIds() which is marked with @Deprecated // TODO: Remove after testing.

Am I supposed to make a code change as part of testing?

No. I mentioned this in chat. I left the old code in place (for now) so we can get accurate before and after numbers, to verify the improvement and consistency of all the maps. So yes, it will be slow at the moment, intentionally. Do some searches and then look at the Full-Text Search admin page. "Build Security Filter Old" and "Build Security Filter" are the key timings.

@labkey-jeckels

Copy link
Copy Markdown
Contributor

The profile looks the same and it's still slow. It's spending all of its time in SearchCategory.getPermittedContainerIds() which is marked with @Deprecated // TODO: Remove after testing.
Am I supposed to make a code change as part of testing?

No. I mentioned this in chat. I left the old code in place (for now) so we can get accurate before and after numbers, to verify the improvement and consistency of all the maps. So yes, it will be slow at the moment, intentionally. Do some searches and then look at the Full-Text Search admin page. "Build Security Filter Old" and "Build Security Filter" are the key timings.

I totally missed that (and can't find it now). The new is 10x faster than the old. I didn't test every variant of permissions but confirmed it's still filtering out containers I can't see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants